From 93f5194e72708776783f2d34894c9b42031e116e Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Fri, 11 Sep 2015 11:42:51 +0100 Subject: [PATCH] libxl: clear O_NONBLOCK|O_NDELAY on migration fd and reinstate afterwards The fd passed to us by libvirt for both save and restore has at least O_NONBLOCK set, which libxl does not expect and therefore fails to handle any EAGAIN which might arise. This has been observed with migration v2, but if v1 used to work I think that would be just be by luck and/or coincidence. Unix convention (and the principal of least surprise) is usually to ensure that an fd has no "strange" properties, such as being non-blocking, when handing it to another component. However for the convenience of the application arrange instead for libxl to clear any unexpected flags on the file descriptors it is given for save or restore and restore them to their original state at the end. O_NDELAY could be similarly problematic so clear that as well as O_NONBLOCK. To do this introduce a pair of new helper functions one to modify+save the flags and another to restore them and call them in the appropriate places. The migration v1 code appeared to do some things with O_NONBLOCK in the checkpoint case. Migration v2 doesn't seem to do so, and in any case I wouldn't expect it to be relying on libvirt's setting of O_NONBLOCK when xl doesn't use that flag. Signed-off-by: Ian Campbell Acked-by: Ian Jackson Acked-by: Wei Liu Cc: Jim Fehlig Cc: Andrew Cooper Cc: Shriram Rajagopalan Cc: Yang Hongyang --- tools/libxl/libxl.c | 65 ++++++++++++++++++++++++++++++++++++ tools/libxl/libxl_create.c | 23 ++++++++++++- tools/libxl/libxl_internal.h | 13 ++++++++ 3 files changed, 100 insertions(+), 1 deletion(-) diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c index 10d1909ebb..6ebb96e557 100644 --- a/tools/libxl/libxl.c +++ b/tools/libxl/libxl.c @@ -952,6 +952,12 @@ static void domain_suspend_cb(libxl__egc *egc, libxl__domain_suspend_state *dss, int rc) { STATE_AO_GC(dss->ao); + int flrc; + + flrc = libxl__fd_flags_restore(gc, dss->fd, dss->fdfl); + /* If suspend has failed already then report that error not this one. */ + if (flrc && !rc) rc = flrc; + libxl__ao_complete(egc,ao,rc); } @@ -980,6 +986,11 @@ int libxl_domain_suspend(libxl_ctx *ctx, uint32_t domid, int fd, int flags, dss->live = flags & LIBXL_SUSPEND_LIVE; dss->debug = flags & LIBXL_SUSPEND_DEBUG; + rc = libxl__fd_flags_modify_save(gc, dss->fd, + ~(O_NONBLOCK|O_NDELAY), 0, + &dss->fdfl); + if (rc < 0) goto out_err; + libxl__domain_save(egc, dss); return AO_INPROGRESS; @@ -6507,6 +6518,60 @@ int libxl_fd_set_cloexec(libxl_ctx *ctx, int fd, int cloexec) int libxl_fd_set_nonblock(libxl_ctx *ctx, int fd, int nonblock) { return fd_set_flags(ctx,fd, F_GETFL,F_SETFL,"FL", O_NONBLOCK, nonblock); } +int libxl__fd_flags_modify_save(libxl__gc *gc, int fd, + int mask, int val, int *r_oldflags) +{ + int rc, ret, fdfl; + + fdfl = fcntl(fd, F_GETFL); + if (fdfl < 0) { + LOGE(ERROR, "failed to fcntl.F_GETFL for fd %d", fd); + rc = ERROR_FAIL; + goto out_err; + } + + LOG(DEBUG, "fnctl F_GETFL flags for fd %d are %x", fd, fdfl); + + if (r_oldflags) + *r_oldflags = fdfl; + + fdfl &= mask; + fdfl |= val; + + LOG(DEBUG, "fnctl F_SETFL of fd %d to %x", fd, fdfl); + + ret = fcntl(fd, F_SETFL, fdfl); + if (ret < 0) { + LOGE(ERROR, "failed to fcntl.F_SETFL for fd %d", fd); + rc = ERROR_FAIL; + goto out_err; + } + + rc = 0; + +out_err: + return rc; +} + +int libxl__fd_flags_restore(libxl__gc *gc, int fd, int fdfl) +{ + int ret, rc; + + LOG(DEBUG, "fnctl F_SETFL of fd %d to %x", fd, fdfl); + + ret = fcntl(fd, F_SETFL, fdfl); + if (ret < 0) { + LOGE(ERROR, "failed to fcntl.F_SETFL for fd %x", fd); + rc = ERROR_FAIL; + goto out_err; + } + + rc = 0; + +out_err: + return rc; + +} void libxl_hwcap_copy(libxl_ctx *ctx,libxl_hwcap *dst, libxl_hwcap *src) { diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c index 29c2641e09..f5771da6ac 100644 --- a/tools/libxl/libxl_create.c +++ b/tools/libxl/libxl_create.c @@ -1556,6 +1556,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, { AO_CREATE(ctx, 0, ao_how); libxl__app_domain_create_state *cdcs; + int rc; GCNEW(cdcs); cdcs->dcs.ao = ao; @@ -1563,8 +1564,13 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, libxl_domain_config_init(&cdcs->dcs.guest_config_saved); libxl_domain_config_copy(ctx, &cdcs->dcs.guest_config_saved, d_config); cdcs->dcs.restore_fd = cdcs->dcs.libxc_fd = restore_fd; - if (restore_fd > -1) + if (restore_fd > -1) { cdcs->dcs.restore_params = *params; + rc = libxl__fd_flags_modify_save(gc, cdcs->dcs.restore_fd, + ~(O_NONBLOCK|O_NDELAY), 0, + &cdcs->dcs.restore_fdfl); + if (rc < 0) goto out_err; + } cdcs->dcs.callback = domain_create_cb; libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how); cdcs->domid_out = domid; @@ -1572,6 +1578,10 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, initiate_domain_create(egc, &cdcs->dcs); return AO_INPROGRESS; + + out_err: + return AO_CREATE_FAIL(rc); + } static void domain_create_cb(libxl__egc *egc, @@ -1579,10 +1589,21 @@ static void domain_create_cb(libxl__egc *egc, int rc, uint32_t domid) { libxl__app_domain_create_state *cdcs = CONTAINER_OF(dcs, *cdcs, dcs); + int flrc; STATE_AO_GC(cdcs->dcs.ao); *cdcs->domid_out = domid; + if (dcs->restore_fd > -1) { + flrc = libxl__fd_flags_restore(gc, + dcs->restore_fd, dcs->restore_fdfl); + /* + * If restore has failed already then report that error not + * this one. + */ + if (flrc && !rc) rc = flrc; + } + libxl__ao_complete(egc, ao, rc); } diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h index c3c3b7ba53..5fa55a770c 100644 --- a/tools/libxl/libxl_internal.h +++ b/tools/libxl/libxl_internal.h @@ -631,6 +631,17 @@ _hidden int libxl__pipe_nonblock(libxl_ctx *ctx, int fds[2]); * `not open'. Ignores any errors. Sets fds[] to -1. */ _hidden void libxl__pipe_close(int fds[2]); +/* Change the flags for the file description associated with fd to + * (flags & mask) | val. + * If r_oldflags != NULL then sets *r_oldflags to the original set of + * flags. + */ +_hidden int libxl__fd_flags_modify_save(libxl__gc *gc, int fd, + int mask, int val, int *r_oldflags); +/* Restores the flags for the file description associated with fd to + * to the previous value (returned by libxl__fd_flags_modify_save) + */ +_hidden int libxl__fd_flags_restore(libxl__gc *gc, int fd, int old_flags); /* Each of these logs errors and returns a libxl error code. * They do not mind if path is already removed. @@ -3050,6 +3061,7 @@ struct libxl__domain_suspend_state { uint32_t domid; int fd; + int fdfl; /* original flags on fd */ libxl_domain_type type; int live; int debug; @@ -3391,6 +3403,7 @@ struct libxl__domain_create_state { libxl_domain_config *guest_config; libxl_domain_config guest_config_saved; /* vanilla config */ int restore_fd, libxc_fd; + int restore_fdfl; /* original flags of restore_fd */ libxl_domain_restore_params restore_params; libxl__domain_create_cb *callback; libxl_asyncprogress_how aop_console_how; -- 2.30.2